[NAE-2446] Migration for NAE v7 phase 1#448
Conversation
Added comprehensive test cases for the `nae_2432` process migration. Introduced `MigrationHelper` to facilitate data field addition, transition updates, and error handling during migrations. Created `MigrationError` to encapsulate migration failure states with detailed metadata. Updated PetriNet versions from v1.0 to v2.0 and verified seamless case migration, role updates, and task modifications.
…ifier` for PetriNet retrieval
Update migration tests: rename PetriNet files and identifiers from `nae_2432` to `migration_test`
Remove unused `webEnvironment` property from Elastic test annotations
- Add support for case removal synchronized between MongoDB and Elasticsearch in `MigrationHelper` and `CaseMigrationHelper`. - Introduce `updateCasesCursor` for ObjectId-based case updates. - Refactor and streamline `migratePetriNet` logic to update case IDs. - Implement MongoDB bulk operations with upsert using `FindAndReplaceOptions`. - Update migration tests with constants, improved assertions, and enhanced logic.
|
Warning Review limit reached
More reviews will be available in 34 minutes and 45 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (16)
WalkthroughThis PR introduces a comprehensive MongoDB migration framework for bulk document operations on Cases, Tasks, and Petri Nets. It includes configurable error handling policies, QueryDSL predicate support, pagination with cursor streaming, and Elasticsearch integration for case/task indexing. The framework provides a public facade ( ChangesMigration System Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/AbstractMigrationHelper.groovy`:
- Around line 53-58: Update the field Javadoc for migrationErrors in
AbstractMigrationHelper to reflect that it is a List<MigrationError> (not a
map): describe that migrationErrors is a collection holding MigrationError
entries encountered during migration, each representing an individual migration
failure; remove references to keys, string identifiers, and lists-per-key, and
adjust any thread-safety wording to match the actual implementation (e.g., note
whether the list is synchronized or not) so the comment accurately matches the
migrationErrors field.
- Around line 192-194: The Javadoc block in AbstractMigrationHelper is malformed
because the first line of the comment lacks the leading "*" and runs into the
next line; update the Javadoc for the method (the comment starting at the block
above the iterator/bulk-operation method in class AbstractMigrationHelper) by
adding the missing "*" at the start of the first line so each line begins with
"*" and the comment renders correctly as a standard Javadoc block.
- Around line 226-266: The outer catch in the iterate loop is re-invoking
handleMigrationError for exceptions that have already been routed (and possibly
re-thrown) by prepareOperations or effectiveProcessOperations, which causes
double-handling and masks original errors; modify the catch block around the
cursor iteration so it checks for MigrationErrorException (or a marker that
handleMigrationError already threw) and rethrows it immediately without calling
handleMigrationError again, and only call handleMigrationError for other
unexpected exceptions (so keep references to prepareOperations,
effectiveProcessOperations, handleMigrationError, MigrationErrorException and
finishMigrationErrorPolicy to locate the logic).
- Around line 317-321: The THROW_AFTER_LIMIT branch incorrectly throws
immediately when policy.maxErrors == 0 and bypasses throwOriginal; change the
logic in the MigrationErrorHandlingMode.THROW_AFTER_LIMIT case to first treat
maxErrors==0 as "no limit" (i.e. only apply the limit when policy.maxErrors > 0)
and when the limit is reached call the central throwError(...) path instead of
directly throwing new MigrationErrorException so throwOriginal and cacheErrors
behavior are honored; specifically update the condition around
getErrors().size() and policy.maxErrors to require policy.maxErrors > 0 and
route handling through throwError(...) (respecting cacheErrors and existing
error caching) rather than constructing/throwing MigrationErrorException inline.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`:
- Around line 448-450: The error logging and error handler call in removeCase
are mislabelled: update the message string and the handleMigrationError
operation parameter to reference "removeCase" (or "removeCase: delete case"
phrasing) instead of "migratePetriNet"; specifically modify the local variable
message and the handleMigrationError(errorPolicy, "migratePetriNet", type,
useCase.stringId, message) invocation in the removeCase method so both
accurately reflect removeCase while keeping errorPolicy, type and
useCase.stringId unchanged.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/MigrationProperties.java`:
- Around line 143-147: The docs say maxErrors == 0 means "no limit" but the
handler throws immediately because it checks getErrors().size() >=
policy.maxErrors; update AbstractMigrationHelper.handleMigrationError to treat 0
as unlimited by changing the throw condition to only apply when policy.maxErrors
> 0 (e.g., require policy.maxErrors > 0 && getErrors().size() >=
policy.maxErrors), keeping existing behavior for positive limits and preserving
references to getErrors(), policy.maxErrors, and the THROW_AFTER_LIMIT policy.
In
`@application-engine/src/main/java/com/netgrif/application/engine/utils/MongodbUtils.java`:
- Around line 9-15: MongodbUtils currently exposes only the static method
toQuery but is instantiable; make the intent explicit by adding a private
constructor to MongodbUtils (e.g., private MongodbUtils() { throw new
AssertionError("No instances."); } or simply private MongodbUtils() {} ) so
callers cannot instantiate the class; update class MongodbUtils to include this
private constructor while leaving the static toQuery(MongoTemplate, Class<T>,
Predicate...) unchanged.
In
`@application-engine/src/test/groovy/com/netgrif/application/engine/migration/MigrationTest.groovy`:
- Around line 69-104: The beforeEach setup resets the DBs but not the
centralized migration error cache, causing flaky tests; add a call to clear the
error cache (e.g., migrationHelper.clearErrors() or the appropriate
clearErrors() method on the migration helper) immediately after
testHelper.truncateDbs() inside the beforeEach method so the shared error state
is reset before importing nets and creating cases, ensuring
migrationHelper.hasErrors() checks in other tests are not impacted by prior
runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ceca0d7-f7c3-4af9-9ed4-eec4c19742fd
📒 Files selected for processing (16)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/MigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/AbstractMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/TaskMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationError.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorHandlingMode.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorPolicy.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/throwable/MigrationErrorException.groovyapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/MigrationProperties.javaapplication-engine/src/main/java/com/netgrif/application/engine/utils/MongodbUtils.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticSearchTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovyapplication-engine/src/test/groovy/com/netgrif/application/engine/migration/MigrationTest.groovyapplication-engine/src/test/resources/petriNets/migration_test_v1.xmlapplication-engine/src/test/resources/petriNets/migration_test_v2.xml
💤 Files with no reviewable changes (2)
- application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticSearchTest.groovy
- application-engine/src/test/groovy/com/netgrif/application/engine/elastic/ElasticTaskTest.groovy
- Update error handling logic in `CaseMigrationHelper` and `AbstractMigrationHelper`, including better handling of migration limits and error policies. - Replace error map with a thread-safe `CopyOnWriteArrayList` in migration processes. - Add safeguards for `MigrationErrorException` handling. - Make `MongodbUtils` a utility class with a private constructor to prevent instantiation. - Enhance migration tests with error clearing before execution.
- Correct parameter names and error messages in `CaseMigrationHelper` and `AbstractMigrationHelper`. - Simplify query and bulk operation processing logic with early return on empty counts. - Update role reference replacement methods: rename `replaceUserFieldRoleReferences` to `replaceActorFieldRoleReferences`. - Adjust `MigrationErrorPolicy` logic to allow zero error limits. - Enhance `PetriNetMigrationHelper` to utilize `ImportPetriNetParams` for reimport functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy (1)
190-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull check is ineffective—NPE thrown before it executes.
Line 193 passes
petriNetService.get(useCase.petriNetObjectId)tosetPetriNet. If the service returnsnull,PetriNetMigrationHelper.setPetriNetwill throw aNullPointerExceptionwhen constructing the newPetriNet(net)at line 487 ofPetriNetMigrationHelper—before the null check at line 194 can execute.Move the null check before calling
setPetriNet:🐛 Proposed fix
void elasticIndex(Case useCase, MigrationErrorPolicy errorPolicy = defaultErrorPolicy()) { log.debug("Starting elasticIndex for case: ${useCase.stringId}") try { - PetriNetMigrationHelper.setPetriNet(useCase, petriNetService.get(useCase.petriNetObjectId)) - if (!useCase.petriNet) { - String message = "Failed to set petriNet for case $useCase.stringId" + PetriNet net = petriNetService.get(useCase.petriNetObjectId) + if (!net) { + String message = "PetriNet not found for case $useCase.stringId (petriNetObjectId: $useCase.petriNetObjectId)" log.error(message) handleMigrationError(errorPolicy, "elasticIndex", type, useCase.stringId, message) return } + PetriNetMigrationHelper.setPetriNet(useCase, net) log.trace("Successfully set petriNet for case: ${useCase.stringId}") elasticCaseService.indexNow(elasticCaseMappingService.transform(useCase))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy` around lines 190 - 219, The null check at line 194 in the elasticIndex method executes too late to prevent a NullPointerException. The issue is that petriNetService.get(useCase.petriNetObjectId) is passed directly to PetriNetMigrationHelper.setPetriNet at line 193, and if the service returns null, setPetriNet will throw an exception before the null check can run. Move the null check before calling setPetriNet by first storing the result of petriNetService.get(useCase.petriNetObjectId) in a variable, checking if it is null, and only then calling PetriNetMigrationHelper.setPetriNet with that non-null result. Keep the existing error handling logic intact.application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy (1)
300-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWarning message references wrong entity.
Line 303 logs "Transition with importId $transitionId not found" but the code is checking whether the role (with
roleImportId) was found, not the transition. The transition lookup happens later in the delegated method at line 285.📝 Proposed fix
static void updateTransitionRoles(PetriNet net, String transitionId, String roleImportId, Map<String, Boolean> permissions) { ProcessRole role = net.roles.values().find { it.importId == roleImportId } if (!role) { - log.warn("Transition with importId $transitionId not found in net $net.identifier") + log.warn("Role with importId $roleImportId not found in net $net.identifier") return } updateTransitionRoles(net, transitionId, role, permissions) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy` around lines 300 - 307, The warning log message in the updateTransitionRoles method is logging incorrect information. The code checks whether a ProcessRole with the given roleImportId exists, but the warning message incorrectly states "Transition with importId $transitionId not found". Update the log.warn message to correctly indicate that the role with roleImportId was not found, not the transition. Change the message to reference roleImportId and clarify that it is the role that could not be found in the net.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`:
- Line 343: The JavaDoc comment for the toRemove parameter in the
CaseMigrationHelper class incorrectly describes the operation as "add" when it
should describe the operation as "remove". Update the `@param` toRemove
documentation to accurately reflect that the parameter contains a map of field
IDs to lists of choices to remove from the data field, not add.
---
Outside diff comments:
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovy`:
- Around line 190-219: The null check at line 194 in the elasticIndex method
executes too late to prevent a NullPointerException. The issue is that
petriNetService.get(useCase.petriNetObjectId) is passed directly to
PetriNetMigrationHelper.setPetriNet at line 193, and if the service returns
null, setPetriNet will throw an exception before the null check can run. Move
the null check before calling setPetriNet by first storing the result of
petriNetService.get(useCase.petriNetObjectId) in a variable, checking if it is
null, and only then calling PetriNetMigrationHelper.setPetriNet with that
non-null result. Keep the existing error handling logic intact.
In
`@application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovy`:
- Around line 300-307: The warning log message in the updateTransitionRoles
method is logging incorrect information. The code checks whether a ProcessRole
with the given roleImportId exists, but the warning message incorrectly states
"Transition with importId $transitionId not found". Update the log.warn message
to correctly indicate that the role with roleImportId was not found, not the
transition. Change the message to reference roleImportId and clarify that it is
the role that could not be found in the net.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 226ef1e0-308d-40da-bba5-9cf7507640dc
📒 Files selected for processing (7)
application-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/AbstractMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/CaseMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/helpers/PetriNetMigrationHelper.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationError.groovyapplication-engine/src/main/groovy/com/netgrif/application/engine/migration/model/MigrationErrorPolicy.groovyapplication-engine/src/main/java/com/netgrif/application/engine/utils/MongodbUtils.javaapplication-engine/src/test/groovy/com/netgrif/application/engine/migration/MigrationTest.groovy
|
@coderabbitai can you please check the PR once more? please approve if it is ok |
|
✅ Action performedFull review finished. |
|
@coderabbitai you wrote full review finished, but you did nothing - no additional comments and no approve. I solved everything you suggested, so if you do not have any additional questions/concerns, then approve it! |
|
(ʘ‿ʘ)ノ🐇 ✅ Action performedComments resolved and changes approved. |
Description
Implements NAE-2446
Dependencies
No new dependencies were introduced
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Tests